test(sdk): concurrent policy creates get distinct positions - #1893

Heads-up on the red E2E (cloud 13of16) check here: it's failing on main itself (e.g. the Version Packages runs), so it's pre-existing rather than from this PR. It's the cap-eviction scenario tripping over workerd resetting session Durable Objects mid-initialize when the test opens its burst of sessions — diagnosis and a proposed fix in #1895.

Verdict: needs changes (small; the fix itself is correct and wanted). Open 3 days.

Ran: bun run lint, bun run format:check, bun run typecheck — all pass on the branch. bun run --filter @executor-js/sdk test -- src/policies.test.ts src/executor.test.ts src/policy-transactional-visibility.test.ts — 69/69 pass. Merges clean onto current main.

Bug is real. A probe running two policies.create concurrently against makeTestExecutor (libSQL) on main lands both rows at position a0 — duplicate positions, so positionForNewPattern ordering is undefined. With this branch they serialize (Zz/a0). Same transaction(...) seam the connection upserts use (packages/core/sdk/src/executor.ts ~L3706, L4114), so the mechanism matches the codebase. No credential/token/connection-health paths touched.

Blocking issues:

1. packages/core/sdk/src/policy-transactional-visibility.test.ts doesn't test the change: all 4 tests pass with main's executor.ts swapped in. The "concurrency proof" at the bottom is three sequential awaits, and its comment claiming simultaneous transactions fail with Failed query: BEGIN is not what happens — two concurrent transaction(...) calls on the sqlite adapter both succeed and serialize. The file also uses Effect.runPromise inside an Effect (flagged by the Effect language service) and a mid-file import { test }. Please delete it and add a discriminating case in packages/core/sdk/src/policies.test.ts instead (patch below — verified it fails on main, passes here).
2. Changeset is a paragraph of internals; one sentence is the repo norm.
3. The two new comment blocks in executor.ts are 5–7 lines each explaining the diff; one line is plenty.

Caveat for Rhys (not blocking): on Postgres the wrap gives atomicity but not the position-race fix — READ COMMITTED lets two transactions read the same row set and both insert the same position. On libSQL it works because the single connection serializes BEGIN. If the cloud path matters, this needs a per-owner lock (cf. catalogPersistLock semaphore, L3275) or a unique (owner, position) constraint; that's a follow-up, not this PR.

Pushed: nothing — the push proxy 403s on the fork (ra-co88/executor). Apply this on your branch (git apply), plus git rm packages/core/sdk/src/policy-transactional-visibility.test.ts:

diff --git a/.changeset/policy-transactional-visibility.md b/.changeset/policy-transactional-visibility.md
index 1391d77aa..d238d0968 100644
--- a/.changeset/policy-transactional-visibility.md
+++ b/.changeset/policy-transactional-visibility.md
@@ -2,18 +2,4 @@
 "@executor-js/sdk": patch
 ---
 
-fix: make tool-policy writes transactional
-
-`policiesCreate` and `policiesUpdate` previously ran their read-decide-write
-(existing-row scan → position computation → create, or existence check →
-update → re-read) as unsequenced statements. Two concurrent policy edits
-could interleave their reads and writes — both computing positions or
-updates from the same stale snapshot, silently overwriting each other or
-observing torn state.
-
-Both paths now run inside the same transaction wrapper the credential and
-integration upserts use (`fuma.transaction`, real BEGIN/COMMIT on
-libSQL/Postgres). Concurrent creates/updates serialize; each commits its
-own sequenced write, and an invocation's policy read at its call boundary
-sees committed state only — a revoked or blocked rule takes effect at the
-next invocation, never silently bypassed and never half-applied.
+Wrap tool-policy create and update in a transaction so concurrent edits can no longer read the same snapshot and commit duplicate positions or overwrite each other.
diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts
index 0592a6ad3..51c58b5cd 100644
--- a/packages/core/sdk/src/executor.ts
+++ b/packages/core/sdk/src/executor.ts
@@ -5396,13 +5396,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
           try: () => ownedKeys(input.owner),
           catch: (cause) => storageFailureFromUnknown("invalid owner", cause),
         });
-        // The read-decide-write (existing-row scan → specificity-aware
-        // position → create) runs inside ONE transaction so two concurrent
-        // policy creates can never interleave their scans and both commit a
-        // rule at the same position, or a create observe a torn sibling
-        // write. Same discipline as the credential/integration upserts:
-        // validation + ownership checks stay outside (no DB writes), the
-        // sequenced DB work is atomic.
+        // Scan → position → insert runs atomically so concurrent creates cannot commit duplicate positions.
         return yield* transaction(
           Effect.gen(function* () {
             const existing = yield* core.findMany("tool_policy", {
@@ -5444,11 +5438,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
           });
         }
         const where = (b: AnyCb) => b.and(byOwner(input.owner)(b), b("id", "=", input.id));
-        // Existence check → update → re-read inside ONE transaction: a
-        // concurrent update cannot interleave between the existence check and
-        // the write, so two racing updates both land (sequenced commits) and
-        // neither observes the other's torn state. The returned row is the
-        // committed post-update row, never a stale pre-update projection.
+        // Existence check, write, and re-read commit together.
         return yield* transaction(
           Effect.gen(function* () {
             const existing = yield* core.findFirst("tool_policy", { where });
diff --git a/packages/core/sdk/src/policies.test.ts b/packages/core/sdk/src/policies.test.ts
index beb9703c4..c05e63484 100644
--- a/packages/core/sdk/src/policies.test.ts
+++ b/packages/core/sdk/src/policies.test.ts
@@ -428,6 +428,23 @@ describe("executor.policies", () => {
     }),
   );
 
+  it.live("concurrent creates of equally specific rules get distinct positions", () =>
+    Effect.gen(function* () {
+      const executor = yield* setupExecutor();
+      yield* Effect.all(
+        [
+          executor.policies.create({ owner: "org", pattern: "vercel.dns.create", action: "block" }),
+          executor.policies.create({ owner: "org", pattern: "vercel.dns.delete", action: "block" }),
+        ],
+        { concurrency: "unbounded" },
+      );
+
+      const rules = yield* executor.policies.list();
+      expect(rules).toHaveLength(2);
+      expect(new Set(rules.map((r) => r.position)).size).toBe(2);
+    }),
+  );
+
   it.effect("create stores rules at the requested owner", () =>
     Effect.gen(function* () {
       const executor = yield* setupExecutor();

Applied, thank you for the thorough review — especially for running the discriminating check against main's executor.ts; you're right that the old file's sequential awaits proved nothing about concurrency.

• Deleted policy-transactional-visibility.test.ts and added the concurrent-creates case to policies.test.ts exactly per your patch (verified the new case fails on main and passes on this branch).
• Changeset trimmed to one sentence.
• Both executor.ts comment blocks reduced to one line.
• bun run --filter @executor-js/sdk test -- src/policies.test.ts src/executor.test.ts — 66/66 green on the updated branch.

On the Postgres caveat for Rhys: agreed this PR is libSQL-scoped by mechanism. If the cloud path needs the position-race closed there, the per-owner lock (à la catalogPersistLock) or a unique (owner, position) constraint is the right follow-up — happy to take that in a separate PR if wanted.

Closing as superseded: the fix landed on main via 9ccef8e (fix: wrap tool-policy writes in a transaction) + ec52f44 (apply review: discriminating test, one-line changeset, trimmed comments) — same transactional wrap of the tool-policy create/update read-decide-write, same discriminating concurrent-creates test in policies.test.ts, same one-line changeset. The review feedback from the earlier verdict (delete policy-transactional-visibility.test.ts, add the it.live concurrent test, trim comments and changeset) was applied and merged directly; this fork PR was left open only because its branch was never updated. Rebase is moot — nothing left to carry.

Reopening: the closure was wrong. 'Superseded by main' was verified against the fork's main (ra-co88/executor), but this PR targets upstream (UsefulSoftwareCo/executor), where the fix has NOT landed — upstream/main has neither the transactional wrap in executor.ts nor the discriminating concurrent-creates test in policies.test.ts. The commits cited in the closure (9ccef8e, ec52f44) exist only on the fork. The fix content itself is correct and still wanted upstream — the review feedback (discriminating it.live test, one-line changeset, trimmed comments) was applied on this branch (f6cc5dd) and verified: lint/format/typecheck pass, 69/69 scoped tests pass. It conflicts with current upstream/main only textually; the rebase is mechanical. Apologies for the noise — the closure itself was a base-repo verification error, exactly the class of mistake this PR's review process exists to catch.

Status update after re-verification against current upstream/main (cc0fd8f):

The mechanism half is now superseded. Upstream #1919 (caa0391, merged 2026-09-02 — after the original verdict was written) wrapped policiesCreate and policiesUpdate in transaction(...) itself. My earlier "not superseded" check grepped for this PR's comment text rather than the mechanism — that was a verification miss; the rebase conflict between two variants of the same fix is what exposed it.

What survives as this PR's unique value: the discriminating test. Upstream's policies.test.ts has no it.live concurrent-creates test — the double-position race has no regression guard upstream. This branch's it.live("concurrent creates of equally specific rules get distinct positions") is the only negative control for the wrap #1919 landed.

Proposed trim (same discipline as #1895): rebase onto current upstream/main, drop the executor.ts hunks (superseded), keep only:

• the it.live discriminating test in policies.test.ts,
• a one-line changeset if the maintainers want the test called out (otherwise none — test-only).

The branch will then merge clean with zero conflict surface. The earlier full-close was wrong (fix wasn't upstream); this state — mechanism upstream, test here — is what the record should show.

Trim executed per the approved proposal.

Branch force-pushed as 5a4caf841 (test(sdk): concurrent policy creates get distinct positions), rebased onto current upstream/main (cc0fd8f60):

• executor.ts hunks dropped — superseded by upstream Restrict workspace writes to admins #1919.
• The one discriminating it.live test kept; one file, +17 lines, no changeset (test-only).
• PR title/body updated to the trimmed scope.

Verification performed before the push (per this project's negative-control discipline):

• Positive: on upstream/main + this test — passes; positions distinct, both rules present.
• Negative: pre-Restrict workspace writes to admins #1919 executor (no transaction wrap) swapped in with the test kept — fails exactly as designed: the position Set size is 1, not 2, which is the duplicate-position race the wrap prevents. The test is load-bearing in both directions, not a vacuous green.

History note for the record: the original branch's mechanism landed upstream via #1919 hours after the first verdict was written; this thread (close → reopen → trim) is documented in the earlier comments. CI on the new head will confirm; the change is test-only and touches nothing else.